๐ก๏ธ Sentinel: [CRITICAL] TimeoutExpired ์์ธ์ ์ ๋ณด ์ ์ถ ์ทจ์ฝ์ ์์ (Fix information disclosure in subprocess timeout handling)#603
Conversation
|
๐ Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a ๐ emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
ec95724 to
cc85a7e
Compare
There was a problem hiding this comment.
Pull request overview
Updates the scripts/ci/sandboxed_verify.py timeout-handling path to redact sensitive data from subprocess.TimeoutExpired output before printing, preventing secret leakage into CI logs. This aligns with the repoโs broader โscrub before loggingโ security posture and adds targeted tests to prevent regressions.
Changes:
- Add regex-based sensitive-data redaction (
SENSITIVE_DATA_SCRUB_PATTERNS+scrub_sensitive_data) and apply it intimeout_output_text. - Ensure
TimeoutExpired.stdout/.stderrare sanitized before being printed on timeout. - Add unit tests covering redaction and timeout output normalization; document the learning in
.jules/sentinel.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
scripts/ci/sandboxed_verify.py |
Introduces secret-scrubbing and applies it to timeout output handling to prevent log disclosure. |
tests/test_sandboxed_verify.py |
Adds tests validating scrubbing behavior and timeout-output masking. |
.jules/sentinel.md |
Records the security learning and prevention guidance for future work. |
๐ก Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SENSITIVE_DATA_SCRUB_PATTERNS = ( | ||
| (re.compile(r'(?i)(bearer\s+)[^\s"\'\\]+'), r'\1***'), | ||
| (re.compile(r'(?i)(token\s+)[^\s"\'\\]+'), r'\1***'), | ||
| (re.compile(r'(?i)\b(?:github_pat_[A-Za-z0-9_]+|gh[pousr]_[A-Za-z0-9_]+)\b'), '***'), | ||
| (re.compile(r'\b(sk-[A-Za-z0-9_-]+)'), '***'), |
cc85a7e to
b6fe48e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tests/test_sandboxed_verify.py:101
- This test embeds a contiguous
ghp_...token-like literal. Even if shortened, itโs safer to avoid committing secret-shaped substrings because gitleaks is enforced as a hard gate in this repo. Construct the token string from pieces to keep the regression coverage without risking secret-scan failures.
def test_timeout_output_text_normalizes_subprocess_payloads():
"""Timeout output normalization handles subprocess bytes and missing streams, and masks secrets."""
assert sandboxed_verify.timeout_output_text(None) == ""
assert sandboxed_verify.timeout_output_text(b"byte-output") == "byte-output"
assert sandboxed_verify.timeout_output_text("text-output") == "text-output"
assert sandboxed_verify.timeout_output_text(b"token ghp_1234") == "token ***"
assert sandboxed_verify.timeout_output_text("token ghp_1234") == "token ***"
| def test_scrub_sensitive_data(): | ||
| """Verify that scrub_sensitive_data masks secrets.""" | ||
| assert sandboxed_verify.scrub_sensitive_data(None) is None | ||
| assert sandboxed_verify.scrub_sensitive_data("safe output") == "safe output" | ||
| assert sandboxed_verify.scrub_sensitive_data("token ghp_1234567890abcdef") == "token ***" | ||
| assert sandboxed_verify.scrub_sensitive_data("sk-live-test1234") == "***" | ||
| assert sandboxed_verify.scrub_sensitive_data("xoxb-123-456") == "***" | ||
| assert sandboxed_verify.scrub_sensitive_data("authorization: bearer secret123") == "authorization: bearer ***" | ||
| assert sandboxed_verify.scrub_sensitive_data("AKIAIOSFODNN7EXAMPLE") == "***" |
![pr-auto-rebase[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
subprocess.TimeoutExpired ์์ธ ๊ฐ์ฒด์๋ ํํฐ๋ง๋์ง ์์ stdout/stderr๊ฐ ํฌํจ๋์ด ์์ด ๋ฏผ๊ฐ ์ ๋ณด๊ฐ ์ ์ถ๋ ์ํ์ด ์์ต๋๋ค. sandboxed_verify.py์ timeout_output_text ํจ์์ ์ ๊ท์ ๊ธฐ๋ฐ ๋ฐ์ดํฐ ๋ง์คํน ๋ก์ง์ ์ถ๊ฐํ์ฌ ๋ฏผ๊ฐ ์ ๋ณด ๋ ธ์ถ์ ๋ฐฉ์งํฉ๋๋ค.
b6fe48e to
38055b3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/test_sandboxed_verify.py:92
- The new regression test introduces multiple token-shaped literals (e.g.,
sk-...,xoxb-...,AKIA...) that are likely to be flagged by the orgโs gitleaks-based Secret Scan workflow. The current.gitleaks.tomlallowlist only coverstests/test_pr_review_merge_scheduler.py, so this file is not exempt and CI will likely fail even though the strings are test fixtures. Consider extending the gitleaks allowlist to include this test file (and these specific fixtures), or refactoring the test to avoid token formats that trigger gitleaks while still exercisingscrub_sensitive_data.
assert sandboxed_verify.scrub_sensitive_data("token ghp_1234567890abcdef") == "token ***"
assert sandboxed_verify.scrub_sensitive_data("sk-live-test1234") == "***"
assert sandboxed_verify.scrub_sensitive_data("xoxb-123-456") == "***"
assert sandboxed_verify.scrub_sensitive_data("authorization: bearer secret123") == "authorization: bearer ***"
assert sandboxed_verify.scrub_sensitive_data("AKIAIOSFODNN7EXAMPLE") == "***"
๐จ Severity: CRITICAL
๐ก Vulnerability:
subprocess.TimeoutExpired์์ธ ๊ฐ์ฒด์stdout๊ณผstderr์์ฑ์ ๋ง์คํน ์ฒ๋ฆฌ๋์ง ์์ ์์ ๋ฐ์ดํฐ๋ฅผ ํฌํจํ๋ฏ๋ก ์ด๋ฅผ ๊ทธ๋๋ก ๋ก๊น ํ ๊ฒฝ์ฐ ํ ํฐ ๋ฑ ๋ฏผ๊ฐํ ์ ๋ณด๊ฐ ์ ์ถ๋ ์ํ์ด ์์ต๋๋ค.๐ฏ Impact: ๋ก๊ทธ ํ์ผ์ด๋ ์ฝ์ ์ถ๋ ฅ ๋ฑ์ ํตํด ์ธ์ฆ ํ ํฐ, ๋น๋ฐ๋ฒํธ ๋ฑ์ ์ ๋ณด๊ฐ ๋ ธ์ถ๋ ์ ์์ต๋๋ค.
๐ง Fix:
sandboxed_verify.py์SENSITIVE_DATA_SCRUB_PATTERNS์scrub_sensitive_dataํจ์๋ฅผ ๋์ ํ๊ณ , ์๊ฐ ์ด๊ณผ ๋ฐ์ ์ ์ถ๋ ฅ ํ ์คํธ๋ฅผ ๋ฐํํ๋timeout_output_textํจ์๊ฐ ๋ง์คํน๋ ๋ฌธ์์ด์ ๋ฐํํ๋๋ก ์์ ํ์ต๋๋ค. ํ ์คํธ ์ฝ๋๋ฅผ ์์ฑํด ์๋์ ๊ฒ์ฆํ์ต๋๋ค.โ Verification: ํ ์คํธ, mypy, bandit, interrogate ๋ฑ์ ํต๊ณผ๋ฅผ ํ์ธํ์ต๋๋ค.
PR created automatically by Jules for task 14122590086490778214 started by @seonghobae